core(config): remove gatherer options support #11743
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
connorjclark
reviewed
Dec 1, 2020
lighthouse-core/config/config.js
Outdated
| } else { | ||
| throw new Error('Invalid expanded Gatherer: ' + JSON.stringify(gathererDefn)); | ||
| } | ||
| }); | ||
|
|
||
| const mergedDefns = mergeOptionsOfItems(gathererDefns); | ||
| // De-dupe gatherers by artifact name. | ||
| const mergedDefns = Array.from( |
brendankenny
reviewed
Dec 1, 2020
brendankenny
approved these changes
Dec 1, 2020
There was a problem hiding this comment.
oops, hit submit too soon:
Review stolen from @Beytoven with his permission. LGTM!
Co-authored-by: Brendan Kenny <bckenny@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary (Why)
Gatherer options as implemented today fundamentally clash with the more useful level of artifact options proposed by FR. They aren't being used anywhere in Lighthouse core and I doubt they ever caught on in custom config land (plugins can't define gatherers). Removing them now will ease the config transition plan with minimal cost.
Summary (What)
Removes support for gatherer options. We added them three years ago at the same time as audit options when we first started playing around with LR and originally planned to utilize them for scoped opportunities until we scrapped that plan before I/O that year and we never ended up using them since.
It's a breaking change because custom gatherers might have discovered gatherer options but they aren't particularly useful if you already own the gatherer implementation so I doubt they caught on much.
There's also a small feature improvement here that de-dupes gatherers by artifact name instead of the file path. This would result on overwriting of artifacts at runtime anyway so they're already being de-duped today, but you don't find out about it until much later.
Related Issues/PRs
ref #11313